Skip to content

Bugfix/spring upgrade#217

Merged
Michael7371 merged 11 commits intodevfrom
bugfix/spring-upgrade
Sep 4, 2025
Merged

Bugfix/spring upgrade#217
Michael7371 merged 11 commits intodevfrom
bugfix/spring-upgrade

Conversation

@Michael7371
Copy link

@Michael7371 Michael7371 commented Aug 20, 2025

PR Details

Description

This PR updates the spring boot dependencies for the ODE, and it's primary maven modules from version 3.1.3 to 3.5.4. The primary breaking change was involving the embedded Kafka broker. Outside of that, some dependencies have updated versions to allow for compatibility with spring boot.

Related Issue

Motivation and Context

To keep the ODE up to date with spring boot updates and security fixes.

How Has This Been Tested?

  • Tests run and pass
  • UDP receiver runs properly
  • /tim REST endpoint creates a TIM message on kafka topic topic.OdeTimJsonTMCFiltered

Types of changes

  • Defect fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that cause existing functionality to change)

Checklist:

  • I have added any new packages to the sonar-scanner.properties file
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
    ODE Contributing Guide
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@Michael7371 Michael7371 requested a review from drewjj August 28, 2025 22:13
@Michael7371 Michael7371 marked this pull request as ready for review August 28, 2025 22:13
@Michael7371 Michael7371 requested a review from mcook42 August 28, 2025 22:13
Copy link
Collaborator

@drewjj drewjj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My tests did catch on the TimDepositControllerTest but on a second run all the tests worked. This is similar to how the encoder tests act sometimes so I don't think this is an issue. The majority of the changes seem to be white space changes. One comment regarding a commented out line but that's about it. Nice!

@Michael7371 Michael7371 requested a review from drewjj August 29, 2025 16:42
Copy link
Collaborator

@drewjj drewjj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Comment on lines +43 to +45
if (!started) {
try {
embeddedKafka.kafkaPorts(4242);
embeddedKafka.afterPropertiesSet();
} catch (Exception e) {
throw new KafkaException("Embedded broker failed to start", e);
synchronized (EmbeddedKafkaHolder.class) {
if (!started) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question(blocking): do we need both !started checks, or can one be removed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, this is resolved now.

@mcook42
Copy link

mcook42 commented Aug 29, 2025

issue(blocking): I've gotten the same test failure a few times locally. Have you gotten this same failure? I would refer to the CI build to see if it's an issue with my machine, but it doesn't look like we run tests in the CI anymore.

[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.773 s -- in us.dot.its.jpo.ode.udp.sdsm.SdsmReceiverTest
[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Errors: 
[ERROR]   Asn1DecodedDataRouterTest.testAsn1DecodedDataRouterTIMDataFlow:153 » IllegalState More than one record for topic found
[INFO] 
[ERROR] Tests run: 342, Failures: 0, Errors: 1, Skipped: 0

Copy link

@mcook42 mcook42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me, but I have a blocking question and issue (noted separately). Once those are resolved, I'm good to approve. Thanks for this work!

@mcook42
Copy link

mcook42 commented Aug 29, 2025

issue(blocking): I've gotten the same test failure a few times locally. Have you gotten this same failure? I would refer to the CI build to see if it's an issue with my machine, but it doesn't look like we run tests in the CI anymore.

[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.773 s -- in us.dot.its.jpo.ode.udp.sdsm.SdsmReceiverTest
[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Errors: 
[ERROR]   Asn1DecodedDataRouterTest.testAsn1DecodedDataRouterTIMDataFlow:153 » IllegalState More than one record for topic found
[INFO] 
[ERROR] Tests run: 342, Failures: 0, Errors: 1, Skipped: 0

It looks like this is an existing test failure, so I won't block this PR on this issue

See https://github.com/CDOT-CV/jpo-ode/actions/runs/17335293330/job/49220250166?pr=218

@Michael7371
Copy link
Author

issue(blocking): I've gotten the same test failure a few times locally. Have you gotten this same failure? I would refer to the CI build to see if it's an issue with my machine, but it doesn't look like we run tests in the CI anymore.

[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.773 s -- in us.dot.its.jpo.ode.udp.sdsm.SdsmReceiverTest
[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Errors: 
[ERROR]   Asn1DecodedDataRouterTest.testAsn1DecodedDataRouterTIMDataFlow:153 » IllegalState More than one record for topic found
[INFO] 
[ERROR] Tests run: 342, Failures: 0, Errors: 1, Skipped: 0

It looks like this is an existing test failure, so I won't block this PR on this issue

See https://github.com/CDOT-CV/jpo-ode/actions/runs/17335293330/job/49220250166?pr=218

Interesting, I am not getting that error locally. I am running the tests in PowerShell and not WSL, so maybe that's part of the reason? @drewjj Do you get the error locally?

@Michael7371 Michael7371 merged commit 4d39c9c into dev Sep 4, 2025
4 checks passed
@Michael7371 Michael7371 deleted the bugfix/spring-upgrade branch September 4, 2025 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants